Retry transient upstream failures in PluginHTTPClient - #1284

Codex Review Summary

This comment shows the latest Codex review activity on this pull request.

No actionable comments were generated in the recent review. 🎉

sequenceDiagram
  participant PluginHTTPClient
  participant URLSession
  participant RetrySleeper
  PluginHTTPClient->>URLSession: Send HTTP request
  URLSession-->>PluginHTTPClient: Return response or transport error
  PluginHTTPClient->>RetrySleeper: Sleep for retry backoff
  RetrySleeper-->>PluginHTTPClient: Resume retry loop
  PluginHTTPClient->>URLSession: Send retry request

Merge Risk: 🟡 Moderate · up to 05805

The client now retries transient HTTP failures, but an ambiguous transport failure can still cause a non-idempotent request to be sent twice. Merge readiness is moderate until this duplication risk is addressed or explicitly accepted.

• Fix all pre-merge checks with AI

_{A rabbit checks the retry gate,
Backoff hops from small to great,
Polls and hooks keep paths in line,
Tests record each pause in time,
The SDK rests beneath moonshine.}

_{Comment @coderabbitai help to get the list of available commands.}

Thanks, this was a good catch and the first finding is the one that mattered.

Fixed: .disabled was not the prior behaviour, and I claimed it was. The old code gave one immediate retry to any transient URLError. I narrowed that to stale-pooled-connection codes, which altered pre-existing behaviour rather than adding to it, so a timeout, DNS failure or offline error stopped being retried at all under .disabled. That regressed the three poll loops the opt-out exists to protect: one timeout aborted transcription where it previously advanced to the next iteration. The compatibility retry is unconditional again under both policies, and only the ladder past it is new. Your three poll-loop findings and the HostServices one all resolve to this.

Fixed: laddered transport retries are now gated on idempotent methods. You are right that this was inconsistent. I applied method-awareness to the status set on exactly the reasoning you give, then laddered .timedOut and .networkConnectionLost for any method. The single compatibility retry still applies to every method, as it did before.

Fixed: the 429 grace now checks retryMaxAttempts. It allowed a seventh request when attempt six returned a 429 with an acceptable Retry-After.

Not fixed, and I would rather explain than quietly comply: gating delivered 503 on idempotent methods.

My reasoning for keeping 503 in the any-method set is that it is the one status in the retried group whose semantics say the origin declined to handle the request at all. That is why it sits with 521, 522, 523, 525 and 526 rather than with 502, 504, 520 and 524, where the origin may have received and processed the work. Your submitJob example requires a server that returns 503 after processing, which is a violation of the status's meaning rather than an expected shape.

The cost of gating it is not small: transcription submission is a POST, so 503 is exactly the case a dictation would hit during a provider incident, and method-gating removes most of the value for the primary path.

That said, this is a judgement about your tolerance for a duplicate job in your plugins, not mine, and SpeechmaticsPlugin.submitJob sending multipart with no idempotency key is a fair thing to point at. If you would rather have 503 gated too, say so and I will move it, or add an explicit per-request opt-in for the submission paths that want it.

Test plan unchanged in shape: full SDK suite 762 tests, 3 skipped, 0 failures.

Answering the Codex review as well. I had been filtering PR feedback for one reviewer and missed these three entirely, which is my error, not a disagreement.

Conceded, and fixed in eef4e75: restrict 503 retries to idempotent methods.

I argued the other way when CodeRabbit raised this, on the grounds that 503 semantically means the origin declined to handle the request, which puts it with 522 rather than with 524. Your example is what changed my mind, because it is concrete rather than semantic: AssemblyAIPlugin.submitTranscription sets httpMethod = "POST" and goes through the default policy, so a 503 returned after the job was created would resubmit it. LinearPlugin mutations and OpenAIVectorMemoryPlugin uploads have the same shape. A reading of the RFC does not outweigh a duplicate transcription job.

503 now sits with 502, 504, 520 and 524. The always-safe set keeps 408 and Cloudflare 521, 522, 523, 525 and 526, all of which fail before the origin sees a byte, so the outage that motivated this work is unaffected: it was a 522 on a POST and it is still retried.

Already addressed in 4d3d755: laddering ambiguous POST transport failures. The ladder past the single legacy retry is gated on isIdempotentMethod. Your comment is anchored to 4d3d755 but was written against 9423684; GitHub re-anchored it when the diff moved.

Already addressed in 4d3d755: preserve the legacy retry for disabled pollers. You and CodeRabbit both caught this and you were both right. The compatibility retry now covers the full isTransientNetworkError set under both policies, so .disabled is byte-identical to prior behaviour and the AssemblyAI, Gladia and Speechmatics loops keep their retry.

Full SDK suite after these changes: 764 tests, 3 skipped, 0 failures. Upstream CI is green including the 1,742 app tests.

Confirmed, and thank you for the isolated repro. The finding is right and the mechanism is worse than a missing guard: retryAfterDelay collapses three different cases into one nil, and the caller cannot tell them apart. An absent header, a malformed header, and a valid delta-seconds value above the 86,400 clamp all return nil, and backoffDelay reads nil as "no header" and computes ordinary backoff. So the over-budget case does not just fail to stop, it schedules the same ordinary backoff an absent header would, with the first retry after 0 to 0.5 seconds, when a Retry-After that large is a refusal that should stop the retries.

The comment on maxHonouredRetryAfterSeconds already states the intent I failed to implement: "Anything longer is not a delay, it is a refusal, and we do not sleep on it." We do not sleep on it. We also do not stop, which is the half that was missing.

Worth recording why the suite did not catch it, because the existing test looks like it covers this. testStopsWhenRetryAfterExceedsRemainingBudget uses Retry-After: 600, which is above the 25 second budget but below the 86,400 clamp, so it parses to a real Duration and the retryAfter <= remaining check in backoffDelay correctly stops. The clamp is the defect and 600 sits on the safe side of it. The boundary at 86,400 and 86,401 was untested.

Fixed in 05805cd by making the parse tri-state rather than adding another guard at the call site, so the three cases stay distinguishable:

enum RetryAfterDecision: Equatable {
    case none              // absent or malformed: ladder proceeds unchanged
    case after(Duration)   // valid and within the clamp: existing behaviour
    case refusal           // valid delta-seconds beyond the clamp: stop, return the response
}

The integer parse and the clamp both stay exactly as they are, so the overflow fix is untouched. testValidRetryAfterAboveCeilingRefusesRatherThanRetryingFast pins the boundary at 86,401, asserting exactly one request and no sleep. The existing within-budget test already covers the honoured side.

One scoping note in case it saves you a pass: the 429 path is not affected. isRetryAfterOnlyStatus already guards on let retryAfter and returns the response when the parse yields nil, so an over-clamp 429 stops today. The exposure is the isRetryableStatus ladder only.

Updating the description as well. Two things in it are now false and both are mine to fix: the status table still lists 503 in the any-method row, and the "Smaller decisions" paragraph still describes the first transport retry as narrowed to stale-pooled-connection codes, which 4d3d755 reverted. Rewriting both now.